fix(device): return deep copied pods in GetScheduledPods to prevent data race - #2575
fix(device): return deep copied pods in GetScheduledPods to prevent data race#2575aryansri05 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aryansri05 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthrough
ChangesScheduled pod snapshot
Estimated code review effort: 2 (Simple) | ~5 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/device/pods.go (1)
241-243: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for snapshot independence.
TestGetScheduledPodscurrently checks value equality only. Mutate a returnedPodInfofield, nestedPodfield, andDevicesmap, then verify the manager’s stored value is unchanged. This directly protects the deep-copy contract from regressing to a shallow copy.As per coding guidelines, keep the test runnable with the repository’s
go test ... -short --race -count=1convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/device/pods.go` around lines 241 - 243, Extend TestGetScheduledPods to verify snapshot independence by mutating a returned PodInfo’s field, its nested Pod field, and its Devices map, then assert the manager’s stored pod remains unchanged. Keep the existing equality assertions and ensure the regression test runs under the repository’s go test -short --race -count=1 convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/device/pods.go`:
- Around line 241-243: Extend TestGetScheduledPods to verify snapshot
independence by mutating a returned PodInfo’s field, its nested Pod field, and
its Devices map, then assert the manager’s stored pod remains unchanged. Keep
the existing equality assertions and ensure the regression test runs under the
repository’s go test -short --race -count=1 convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a921595b-f231-4227-9d0a-f650d45a8805
📒 Files selected for processing (1)
pkg/device/pods.go
…ata race GetScheduledPods previously returned a shallow copy of the pods map. Since the values are *PodInfo pointers, the underlying structs were still shared across threads, causing a data race between metrics scraping and scheduler write updates. We now call v.DeepCopy() on the values in the map to return fully independent structs. Signed-off-by: Aryan Srivastava <aryansriva05@gmail.com>
985c224 to
b72f27e
Compare
|
This is being closed because it does not comply with the contribution guidelines. |
|
Apologies for not following the contribution guidelines — I should have disclosed AI assistance upfront and checked for existing PRs before opening this. Won't happen again. |
2 participants
Fixes #2471
Re-opening after #2501 was closed due to an unrelated commit accidentally pushed to the branch (a benchmark graph file, now removed).
Currently,
GetScheduledPods()returns a shallow copy of the pods map viamaps.Copy. While this prevents concurrent map iteration/write panics on the map itself, the map values are*PodInfopointers — the underlying structs are still shared across goroutines.This causes a data race between the metrics scraping goroutine (which reads
PodInfofields) and the scheduler goroutine (which writes to the samePodInfostructs).Fix: call
v.DeepCopy()on each value so callers receive fully independent copies.Signed-off-by: Aryan Srivastava aryansriva05@gmail.com
Summary by CodeRabbit